-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Adding New Isolated Timeout Test Class #45202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
/azp run java - storage - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - storage - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
…to storage/liveTestFailureFixes
…nteraction so it doesnt explode with 429s
/azp run java - storage - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - storage - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - storage - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - storage - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - storage - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - storage - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - storage - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR centralizes timeout behavior tests into dedicated isolated classes and cleans up redundant inline timeout tests across existing API test suites.
- Introduced
TimeoutTests
for synchronous timeout paging scenarios. - Introduced
TimeoutAsyncTests
for asynchronous timeout paging scenarios. - Removed outdated inline timeout tests from
ServiceAsyncApiTests
,ServiceApiTests
,ContainerAsyncApiTests
, andContainerApiTests
.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/TimeoutTests.java | Added new isolated synchronous timeout tests |
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/TimeoutAsyncTests.java | Added new isolated asynchronous timeout tests |
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ServiceAsyncApiTests.java | Removed inline timeout tests |
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ServiceApiTests.java | Removed inline timeout tests |
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ContainerAsyncApiTests.java | Removed inline timeout tests |
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ContainerApiTests.java | Removed inline timeout tests and unused imports |
Comments suppressed due to low confidence (2)
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/TimeoutTests.java:54
- [nitpick] Consider renaming
listBlobsHierWithTimeoutStillBackedByPagedStream
tolistBlobsByHierarchyWithTimeoutStillBackedByPagedStream
for clarity and consistency with the API method name.
public void listBlobsHierWithTimeoutStillBackedByPagedStream() {
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/TimeoutTests.java:39
- [nitpick] Consider adding a negative test case where the timeout is shorter than the page delay (e.g.,
Duration.ofSeconds(3)
) to assert that a timeout exception is thrown as expected.
public void listBlobsFlatWithTimeoutStillBackedByPagedStream() {
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/TimeoutTests.java
Show resolved
Hide resolved
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/TimeoutTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces new isolated timeout tests to verify that individual page requests honor the specified timeout while the overall paged stream remains functional. It also removes several duplicate or legacy timeout tests from other test classes to reduce redundancy and improve test isolation.
- Adds TimeoutTests.java for synchronous client timeout testing.
- Adds TimeoutAsyncTests.java for asynchronous client timeout testing.
- Removes outdated timeout tests from service and container API test classes.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/TimeoutTests.java | New isolated timeout tests for sync clients with custom HttpClient implementations. |
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/TimeoutAsyncTests.java | New isolated timeout tests for async clients using StepVerifier. |
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ServiceAsyncApiTests.java | Removal of legacy timeout tests to avoid redundancy. |
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ServiceApiTests.java | Removal of legacy timeout tests to avoid redundancy. |
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ContainerAsyncApiTests.java | Removal of legacy timeout tests to avoid redundancy. |
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ContainerApiTests.java | Removal of legacy timeout tests to avoid redundancy. |
Comments suppressed due to low confidence (1)
sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/ServiceAsyncApiTests.java:285
- The removal of legacy timeout tests might reduce overall test coverage. Please ensure that the new TimeoutTests and TimeoutAsyncTests fully cover all the critical timeout scenarios previously validated here.
- @Test
- public void listContainersWithTimeoutStillBackedByPagedFlux() {
String delimiterString = useDelimiter ? "<Delimiter>/</Delimiter>" : ""; | ||
|
||
return "<?xml version=\"1.0\" encoding=\"utf-8\"?>" | ||
+ "<EnumerationResults ServiceEndpoint=\"https://account.blob.core.windows.net/\" ContainerName=\"foo\">" | ||
+ "<MaxResults>3</MaxResults>" + delimiterString + "<Blobs>" + "<Blob>" + "<Name>blob1</Name>" | ||
+ "</Blob>" + "<Blob>" + "<Name>blob2</Name>" + "</Blob>" + "<Blob>" + "<Name>blob3</Name>" + "</Blob>" | ||
+ "</Blobs>" + "<NextMarker>MARKER--</NextMarker>" + "</EnumerationResults>"; | ||
} | ||
|
||
private String buildSecondResponse(Boolean useDelimiter) { | ||
String delimiterString = useDelimiter ? "<Delimiter>/</Delimiter>" : ""; | ||
|
||
return "<?xml version=\"1.0\" encoding=\"utf-8\"?>" | ||
+ "<EnumerationResults ServiceEndpoint=\"https://account.blob.core.windows.net/\" ContainerName=\"foo\">" | ||
+ "<Marker>MARKER--</Marker>" + "<MaxResults>3</MaxResults>" + delimiterString + "<Blobs>" + "<Blob>" | ||
+ "<Name>blob4</Name>" + "</Blob>" + "<Blob>" + "<Name>blob5</Name>" + "</Blob>" + "</Blobs>" | ||
+ "<NextMarker/>" + "</EnumerationResults>"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The XML generation methods in the different HttpClient implementations appear similar. Consider extracting shared utilities to build XML responses to improve code maintainability.
String delimiterString = useDelimiter ? "<Delimiter>/</Delimiter>" : ""; | |
return "<?xml version=\"1.0\" encoding=\"utf-8\"?>" | |
+ "<EnumerationResults ServiceEndpoint=\"https://account.blob.core.windows.net/\" ContainerName=\"foo\">" | |
+ "<MaxResults>3</MaxResults>" + delimiterString + "<Blobs>" + "<Blob>" + "<Name>blob1</Name>" | |
+ "</Blob>" + "<Blob>" + "<Name>blob2</Name>" + "</Blob>" + "<Blob>" + "<Name>blob3</Name>" + "</Blob>" | |
+ "</Blobs>" + "<NextMarker>MARKER--</NextMarker>" + "</EnumerationResults>"; | |
} | |
private String buildSecondResponse(Boolean useDelimiter) { | |
String delimiterString = useDelimiter ? "<Delimiter>/</Delimiter>" : ""; | |
return "<?xml version=\"1.0\" encoding=\"utf-8\"?>" | |
+ "<EnumerationResults ServiceEndpoint=\"https://account.blob.core.windows.net/\" ContainerName=\"foo\">" | |
+ "<Marker>MARKER--</Marker>" + "<MaxResults>3</MaxResults>" + delimiterString + "<Blobs>" + "<Blob>" | |
+ "<Name>blob4</Name>" + "</Blob>" + "<Blob>" + "<Name>blob5</Name>" + "</Blob>" + "</Blobs>" | |
+ "<NextMarker/>" + "</EnumerationResults>"; | |
return buildXmlResponse(useDelimiter, new String[]{"blob1", "blob2", "blob3"}, "MARKER--"); | |
} | |
private String buildSecondResponse(Boolean useDelimiter) { | |
return buildXmlResponse(useDelimiter, new String[]{"blob4", "blob5"}, null); |
Copilot uses AI. Check for mistakes.
/azp run java - storage - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
…to storage/liveTestFailureFixes
/azp run java - storage - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good. I just had a couple of suggestions on how we expose or timeout-based HTTP client to make it easier to expand on in the future.
} | ||
|
||
/* | ||
* Used for sync and async tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to move these classes to a shared common base class, a utility class, or a common package? It definitely seems strange that we would have the async tests rely on classes in the sync tests. Generally, having some designated utility location is where I'd expect it to go if we are sharing it between test classes.
} | ||
} | ||
|
||
protected static final class FindBlobsWithTimeoutClient implements HttpClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to consolidate these two client classes into one? Because we are building up our fake responses as part of the send()
, we are not able to do that. But if we refactored the class to be more explicit in the test cases on which responses we build up, I think we could. For example:
// For client list blob case
new TimeoutTestClient()
.addListBlobsResponse(...)
.addListBlobsResponse(...)
// client for find blobs cases
new TimeoutTestClient()
.addFindBlobsResponse(...)
.addFindBlobsResponse(...)
Then each time one of these add
methods were called it would generate an XML response that would get added to some list that returns responses in FIFO order. I think it is worth exploring approach like this because:
- We can reduce some duplication of code
- Simplify the
send()
methods - Make the response generation a more programmatic, instead of hardcoding xml bodies which be difficult to work with if we want to tweak the responses or add more cases.
I'm also open to using different names or having a proper builder for it when it comes to the interface
Separates timeout testing into an isolated test class, as these tests are highly sensitive to resource contention. They were frequently failing due to timeouts being exceeded. The
@Isolated
tag forces JUnit to run them separately from other test files.